Skip to content

Introduce a range-diff viewer for GitHub force-push #2149

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 6 commits into from
Aug 12, 2025

Conversation

Urgau
Copy link
Member

@Urgau Urgau commented Aug 10, 2025

This PR adds an emulated git range-diff viewer for use GitHub and force push.

It's a well known issue with GitHub compare UI that when a force-push changes the base commit that GitHub's compare page will show all changes from the old base to the new head, which includes enormous amount of unrelated changes.

git range-diff solves that by diff showing the diff of the old and new revision, and since GitHub doesn't offer this capability we can do it ourselves.

This new feature works by having a new HTML endpoint /gh-range-diff/{owner}/{repo}/{basehead} that takes the org, repo and base..head, which we then use to retrieve the old and new diff.
Those two diffs are then compared against each other, the resulting differences are then shown to the user.

GitHub compare range-diff
image image
rust-lang/rust#132146 (comment) localhost/range-diff

To access our range-diff viewer users will be able to use the bookmarklet we provide in the page, it redirects from GitHub's compare page to our viewer with the right parameters.

@Urgau Urgau requested a review from Kobzol August 10, 2025 18:26
@Urgau
Copy link
Member Author

Urgau commented Aug 11, 2025

I just pushed a bunch a small changes which should improve the visual clarity, by reducing the highlighting parts and re-using git range-diff block sign to better distinguish the diff of diffs markers from the diffs markers.

I also added before and after links to each files, improved the bookmarklet help and added a small explanation for the block markers. Feedback welcome of course.

before after
image image

Copy link
Member

@Kobzol Kobzol left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Awesome work!

The minus/plus with red/green background is a bit distracting visually, what about just keeping the red/green bar without minus/plus? The background contrast is also a bit dim, I'd either make the background lighter or the font more visible/higher opacity/"more white".

@Urgau
Copy link
Member Author

Urgau commented Aug 12, 2025

The minus/plus with red/green background is a bit distracting visually

For the record, git range-diff actually uses the red/green background with the +/- marker.

what about just keeping the red/green bar without minus/plus?

We can try, but not sure if people will understand what they mean without the +/-.

The background contrast is also a bit dim

The background contrast of the page? Or the red/green colors? Were you in dark mode? In dark mode it's the same background and text color as for our GHA logs viewer.

@Kobzol
Copy link
Member

Kobzol commented Aug 12, 2025

I probably was in dark mode, and yeah, I find the contrast to be not ideal on both pages. But it's not terrible, so definitely a non-blocking comment :) It seems to work well, feel free to merge!

@Urgau
Copy link
Member Author

Urgau commented Aug 12, 2025

Would doing something aking to GitHub diff viewer where the whole line has a background color work better for you?

@Kobzol
Copy link
Member

Kobzol commented Aug 12, 2025

Probably not, I like the current visualization, just wanted to remove the +/- in the very left column, in particular also because the +/- is duplicated on actually changed lines.

@Urgau
Copy link
Member Author

Urgau commented Aug 12, 2025

Well, there is complexity, they are not always duplicated, we can have -+. We can see that on the exemple I gave. It means that the added line from the previous patch is removed.

@Kobzol
Copy link
Member

Kobzol commented Aug 12, 2025

Hmm, I probably don't have experience with range-diff, that's quite confusing to me. I would simply expect to see a diff from the old push to the new push, while ignoring all the other crap that happened on master in the meantime.

@Urgau
Copy link
Member Author

Urgau commented Aug 12, 2025

There isn't any crap left, a range-diff does a diff of diffs, as you would expect.

It's nevertheless quite confusing the first time you read one, as we need to show what was added and removed on the first push vs what was added and removed on the second push.

That's why there is those double markers, the first one indicates what has changed between the the first and second push, and the second marker indicates what was/is the change.

@Kobzol
Copy link
Member

Kobzol commented Aug 12, 2025

If we really wanted to, we could show a normal diff though, right? Just render the old contents and the new contents and diff them, without regarding the fact that the first version was already a "diff".

@Urgau
Copy link
Member Author

Urgau commented Aug 12, 2025

What you're describing is I think what GitHub does, it shows the diff of contents (instead of the diff of diffs), which shows a lot of unrelated changes.

@Kobzol
Copy link
Member

Kobzol commented Aug 12, 2025

Ofc I meant doing a diff between just the relevant changes, not everything.

@Urgau
Copy link
Member Author

Urgau commented Aug 12, 2025

Hum, I guess that could work. We could probably do it by removing the removed lines from the diffs (as well as stripping + from the added lines), that should get us the "final" content limited to the relevant changes of each of them and then diff those together.

@Kobzol
Copy link
Member

Kobzol commented Aug 12, 2025

I'm not saying that it's better, just that I personally find it less confusing. I think the current range diff is fine.

@Urgau
Copy link
Member Author

Urgau commented Aug 12, 2025

The advantage of range-diff is that it gives you the diff between the two push compared to master, while comparing the final relevant parts between each others only gives you the diff between the two push. Each one has his advantages and dis-advantages.

I think the current range diff is fine.

Then let's keep the range-diff for now. We can experiment later with your solution.

@Urgau Urgau added this pull request to the merge queue Aug 12, 2025
Merged via the queue into rust-lang:master with commit b21542f Aug 12, 2025
3 checks passed
@Urgau Urgau deleted the gh-range-diff branch August 12, 2025 17:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants